Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Result in Service #15181

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Remove Result in Service #15181

merged 1 commit into from
Nov 20, 2023

Conversation

ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Nov 18, 2023 •
edited
Loading

Purpose of the pull request

Use Result/Map<String, Object> as method result is not a good practice, this PR will refactor some service method.

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@ruanwenjun ruanwenjun marked this pull request as draft November 18, 2023 04:01
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 18, 2023
View reviewed changes
.../src/main/java/org/apache/dolphinscheduler/api/controller/AlertPluginInstanceController.java
public Result getAlertPluginInstance(@Parameter(hidden = true) @RequestAttribute(value = Constants.SESSION_USER) User loginUser) {
Map<String, Object> result = alertPluginInstanceService.queryAll();
return returnDataList(result);
public Result<List<AlertPluginInstanceVO>> getAlertPluginInstance(@Parameter(hidden = true) @RequestAttribute(value = Constants.SESSION_USER) User loginUser) {

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'loginUser' is never used.
...nscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/AccessTokenService.java
@@ -64,7 +65,7 @@
* @param expireTime token expire time
* @return token string
*/
Map<String, Object> generateToken(User loginUser, int userId, String expireTime);
String generateToken(User loginUser, int userId, String expireTime);

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'loginUser' is never used.
...er-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/AlertGroupServiceImpl.java
}
log.error("Create alert group error, groupName:{}", alertGroup.getGroupName());

Check failure

Code scanning / CodeQL

Log Injection High

This log entry depends on a
user-provided value
Loading
.
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_refactorService branch 2 times, most recently from 405fddf to 3c41548 Compare November 18, 2023 08:31
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2023 •
edited
Loading

Codecov Report

Attention: 167 lines in your changes are missing coverage. Please review.

Comparison is base (6aa6e11) 38.28% compared to head (8b3322d) 37.93%.

❗ Current head 8b3322d differs from pull request most recent head 07a0b0a. Consider uploading reports for the commit 07a0b0a to get more accurate results

Files Patch % Lines
...uler/api/service/impl/DataAnalysisServiceImpl.java 50.00% 19 Missing and 1 partial ⚠️
...eduler/api/service/impl/AlertGroupServiceImpl.java 59.45% 14 Missing and 1 partial ⚠️
...duler/api/service/impl/EnvironmentServiceImpl.java 62.16% 12 Missing and 2 partials ⚠️
...i/service/impl/AlertPluginInstanceServiceImpl.java 48.00% 10 Missing and 3 partials ⚠️
...scheduler/api/service/impl/ProjectServiceImpl.java 13.33% 13 Missing ⚠️
...scheduler/api/controller/DataSourceController.java 23.07% 10 Missing ⚠️
...nscheduler/api/service/impl/DqRuleServiceImpl.java 23.07% 10 Missing ⚠️
...nscheduler/api/service/impl/TenantServiceImpl.java 41.17% 6 Missing and 4 partials ⚠️
...duler/api/service/impl/AccessTokenServiceImpl.java 55.55% 7 Missing and 1 partial ⚠️
...scheduler/api/service/impl/ClusterServiceImpl.java 87.75% 5 Missing and 1 partial ⚠️
... and 18 more
Additional details and impacted files 
@@             Coverage Diff              @@
##                dev   #15181      +/-   ##
============================================
- Coverage     38.28%   37.93%   -0.36%     
+ Complexity     4701     4667      -34     
============================================
  Files          1285     1278       -7     
  Lines         45547    44872     -675     
  Branches       4957     4869      -88     
============================================
- Hits          17436    17020     -416     
+ Misses        26217    25997     -220     
+ Partials       1894     1855      -39

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ruanwenjun ruanwenjun changed the title Remove API Result in Service Remove Result in Service Nov 18, 2023
@ruanwenjun ruanwenjun marked this pull request as ready for review November 18, 2023 08:50
@ruanwenjun ruanwenjun added refactor improvement make more easy to user or prompt friendly labels Nov 18, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 18, 2023
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_refactorService branch 3 times, most recently from 31d95fe to 70beaa2 Compare November 18, 2023 14:29
@Radeity Radeity added the discussion discussion label Nov 18, 2023
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_refactorService branch 6 times, most recently from df34684 to ebcb7c0 Compare November 19, 2023 03:51
@ruanwenjun ruanwenjun mentioned this pull request Nov 19, 2023
Closed
3 tasks
SbloodyS
SbloodyS previously approved these changes Nov 20, 2023
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. This is really a big change. Thanks 👍🏻

@ruanwenjun ruanwenjun added ready-to-merge and removed discussion discussion labels Nov 20, 2023
@ruanwenjun
Copy link
Member Author

Resolve the conflicts.

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 20, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 3 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 55 Code Smells

70.2% 70.2% Coverage
0.3% 0.3% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

caishunfeng
caishunfeng approved these changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall

@ruanwenjun ruanwenjun merged commit 9be81be into apache:dev Nov 20, 2023
51 of 53 checks passed
@ruanwenjun ruanwenjun deleted the dev_wenjun_refactorService branch November 20, 2023 09:04
@SbloodyS SbloodyS mentioned this pull request Feb 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caishunfeng caishunfeng caishunfeng approved these changes

@Radeity Radeity Awaiting requested review from Radeity

@zhongjiajie zhongjiajie Awaiting requested review from zhongjiajie

@EricGao888 EricGao888 Awaiting requested review from EricGao888 EricGao888 is a code owner

@SbloodyS SbloodyS Awaiting requested review from SbloodyS SbloodyS is a code owner

Assignees

@ruanwenjun ruanwenjun

Labels
backend improvement make more easy to user or prompt friendly ready-to-merge refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ruanwenjun @codecov-commenter @caishunfeng @SbloodyS @Radeity